Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Exclude the bind -x commands from interactive commands #152

Merged
merged 2 commits into from
Feb 25, 2024
Merged

Exclude the bind -x commands from interactive commands #152

merged 2 commits into from
Feb 25, 2024

Conversation

akinomyoga
Copy link
Contributor

@akinomyoga akinomyoga commented Feb 16, 2024 •
edited
Loading

Issue: preexec is called for keybindings set up by bind -x

Consider the following setup:

# bashrc

source bash-preexec.sh
function preexec { echo "==== $FUNCNAME ($1) ===="; }
function precmd { echo "==== $FUNCNAME ===="; }

function something { return 0; }
bind -x '"\C-t": something'

When I start a session with the above rcfile (bashrc.exclude-bind-x) and input "echo 1" RET C-t "echo 2" RET C-d, the result becomes this:

[murase@chatoyancy 1 bash-preexec]$ bash --rcfile bashrc.exclude-bind-x
==== precmd ====
[murase@chatoyancy 0 bash-preexec]$ echo 1
==== preexec (echo 1) ====
1
==== precmd ====
==== preexec (echo 1) ====
[murase@chatoyancy 0 bash-preexec]$ echo 2
2
==== precmd ====
[murase@chatoyancy 0 bash-preexec]$
exit

The second preexec is supposed to be called against the second command echo 2, but it's instead called for the command of the keybinding something, which is not an expected behavior.

This problem was identified based on the report at atuinsh/atuin#1003 (comment).

Solution

We can take the same solution as completion functions. For the completion functions, we are using COMP_LINE to test whether we are currently running the completion function. For the bind -x commands, we can test READLINE_POINT to test whether we are running a command for keybindings.

This solution can be broken when the user sets the shell variable READLINE_POINT, but it's not a problem specific to this PR. The existing code for COMP_LINE has the same problem when the user sets the variable. In reality, there shouldn't be many cases where the user sets READLINE_POINT outside the keybinding functions. Rather there are more cases where the users set up keybindings through bind -x.

Here's the behavior after this fix:

[murase@chatoyancy 1 bash-preexec]$ bash --rcfile bashrc.exclude-bind-x
==== precmd ====
[murase@chatoyancy 0 bash-preexec]$ echo 1
==== preexec (echo 1) ====
1
==== precmd ====
[murase@chatoyancy 0 bash-preexec]$ echo 2
==== preexec (echo 2) ====
2
==== precmd ====
[murase@chatoyancy 0 bash-preexec]$
exit

@akinomyoga
Exclude the bind -x commands from interactive commands
a58797c
@akinomyoga
Check COMP_POINT instead of possibly empty COMP_LINE
96d37dc 
The check for COMP_LINE can fail because it can be empty in the
completion function for the empty command line "complete -F _func -E".
@akinomyoga
Copy link
Contributor Author

I pushed another commit, where I suggest changing COMP_LINE to COMP_POINT. The variable COMP_LINE can be empty on the empty command line. For example, when a completion is attempted on the empty command line with the following setting, preexec can be unexpectedly called on pressing TAB.

$ complete -E -F _test1
$ _test1() { declare -p "${!COMP_@}"; }

@akinomyoga akinomyoga mentioned this pull request Feb 17, 2024
Merged
tessus
tessus approved these changes Feb 19, 2024
View reviewed changes
Copy link

@tessus tessus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes perfect sense and certainly fixes the use-case you described. 👍

@akinomyoga
Copy link
Contributor Author

@dimo414 Could you review this PR? This fix is a very small one, so it shouldn't be difficult to review. Or is there anything missing?

@akinomyoga
Copy link
Contributor Author

I thought about if it is possible to add a test, but we need an interactive session to directly test it. Also, the test for the existing COMP_LINE doesn't seem to exist. So I haven't included a test for this PR for now. Maybe we can set up a mock test. Or maybe we can use expect (I've never used it though), but this adds extra dependencies for the test. What do you think?

@rcaloras
Copy link
Owner

@akinomyoga thanks for the PR and fix as always. 👍

Sorry for delays as I've recently had extraneous family responsibilities. Recognize there's a few open issues/PRs the community is trying to push through. I'm interested in trying to get more folks approval access to support.

@rcaloras rcaloras merged commit 8926de0 into rcaloras:master Feb 25, 2024
1 check passed
@akinomyoga akinomyoga deleted the exclude-bind-x branch February 25, 2024 20:18
@akinomyoga
Copy link
Contributor Author

@rcaloras Thank you for taking your time!

I'm interested in trying to get more folks approval access to support.

Maybe we can write something on README.md or CONTRIBUTING.md?

@Rogach Rogach mentioned this pull request Mar 20, 2024
Open
@akinomyoga akinomyoga mentioned this pull request May 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tessus tessus tessus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akinomyoga @rcaloras @tessus